Fully replace Title::moveTo() with MovePage
authorKunal Mehta <legoktm@gmail.com>
Sat, 11 Oct 2014 05:43:02 +0000 (22:43 -0700)
committerKunal Mehta <legoktm@gmail.com>
Tue, 28 Oct 2014 19:52:36 +0000 (12:52 -0700)
* AbortMove hook is removed in favor of two more specificly focused
  hooks: MovePageCheckPermissions and MovePageIsValidMove.
** MovePageIsValidMove is for extensions to specify whether a page
   cannot be moved for technical reasons, and should not be
   overridden.
** MovePageCheckPermissions is for checking whether the given user
   is allowed to make the move.

* Title::moveNoAuth() deprecated
* Title::moveTo() deprecated
* Title::isValidMoveOperation() broken down into
  MovePage::isValidMove() and MovePage::checkPermissions().

* Title::getTitleProtection() is now public, and returns
  unprefixed fields

Change-Id: Ic5026384b92a0d68d628397ffe1de6e5b6183f02

RELEASE-NOTES-1.25
docs/hooks.txt
includes/MovePage.php
includes/Title.php
includes/api/ApiMove.php
includes/specials/SpecialMovepage.php
maintenance/cleanupCaps.php
maintenance/moveBatch.php
tests/phpunit/includes/TitlePermissionTest.php

index 28a3958..161ba40 100644 (file)
@@ -48,6 +48,16 @@ production.
   SVG images in Internet Explorer 6 and 7, as they don't support them anyway.
 * (bug 67021) On Special:BookSources, corrected validation of ISBNs (both
   10- and 13-digit forms) containing "X".
+* Page moving was refactored into a MovePage class. As part of that:
+** The AbortMove hook was removed.
+** MovePageIsValidMove is for extensions to specify whether a page
+   cannot be moved for technical reasons, and should not be overriden.
+** MovePageCheckPermissions is for checking whether the given user is
+   allowed to make the move.
+** Title::moveNoAuth() was deprecated. Use the MovePage class instead.
+** Title::moveTo() was deprecated. Use the MovePage class instead.
+** Title::isValidMoveOperation() broken down into MovePage::isValidMove()
+   and MovePage::checkPermissions().
 
 === Action API changes in 1.25 ===
 * (bug 65403) XML tag highlighting is now only performed for formats
index b71c347..52eeab8 100644 (file)
@@ -260,13 +260,6 @@ $password: the password being submitted, not yet checked for validity
           a machine API rather than the HTML user interface.
 &$msg: the message identifier for abort reason (new in 1.18, not available before 1.18)
 
-'AbortMove': Allows to abort moving an article (title).
-$old: old title
-$nt: new title
-$user: user who is doing the move
-$err: error message
-$reason: the reason for the move (added in 1.13)
-
 'AbortNewAccount': Return false to cancel explicit account creation.
 $user: the User object about to be created (read-only, incomplete)
 &$msg: out parameter: HTML to display on abort
@@ -1835,6 +1828,18 @@ $db: The database object to be queried.
 &$opts: Options for the query.
 &$join_conds: Join conditions for the query.
 
+'MovePageCheckPermissions': Specify whether the user is allowed to move the page.
+$oldTitle: Title object of the current (old) location
+$newTitle: Title object of the new location
+$user: User making the move
+$reason: string of the reason provided by the user
+$status: Status object to pass error messages to
+
+'MovePageIsValidMove': Specify whether a page can be moved for technical reasons.
+$oldTitle: Title object of the current (old) location
+$newTitle: Title object of the new location
+$status: Status object to pass error messages to
+
 'BaseTemplateToolbox': Called by BaseTemplate when building the $toolbox array
 and returning it for the skin to output. You can add items to the toolbox while
 still letting the skin make final decisions on skin-specific markup conventions
index 79095e9..7bad3f9 100644 (file)
@@ -42,6 +42,52 @@ class MovePage {
                $this->newTitle = $newTitle;
        }
 
+       public function checkPermissions( User $user, $reason ) {
+               $status = new Status();
+
+               $errors = wfMergeErrorArrays(
+                       $this->oldTitle->getUserPermissionsErrors( 'move', $user ),
+                       $this->oldTitle->getUserPermissionsErrors( 'edit', $user ),
+                       $this->newTitle->getUserPermissionsErrors( 'move-target', $user ),
+                       $this->newTitle->getUserPermissionsErrors( 'edit', $user )
+               );
+
+               // Convert into a Status object
+               if ( $errors ) {
+                       foreach ( $errors as $error ) {
+                               call_user_func_array( array( $status, 'fatal' ), $error );
+                       }
+               }
+
+               if ( EditPage::matchSummarySpamRegex( $reason ) !== false ) {
+                       // This is kind of lame, won't display nice
+                       $status->fatal( 'spamprotectiontext' );
+               }
+
+               # The move is allowed only if (1) the target doesn't exist, or
+               # (2) the target is a redirect to the source, and has no history
+               # (so we can undo bad moves right after they're done).
+
+               if ( $this->newTitle->getArticleID() ) { # Target exists; check for validity
+                       if ( !$this->isValidMoveTarget() ) {
+                               $status->fatal( 'articleexists' );
+                       }
+               } else {
+                       $tp = $this->newTitle->getTitleProtection();
+                       if ( $tp !== false ) {
+                               if ( !$user->isAllowed( $tp['permission'] ) ) {
+                                       $status->fatal( 'cantmove-titleprotected' );
+                               }
+                       }
+               }
+
+               wfRunHooks( 'MovePageCheckPermissions',
+                       array( $this->oldTitle, $this->newTitle, $user, $reason, $status )
+               );
+
+               return $status;
+       }
+
        /**
         * Does various sanity checks that the move is
         * valid. Only things based on the two titles
@@ -99,6 +145,9 @@ class MovePage {
                        $status->fatal( 'nonfile-cannot-move-to-file' );
                }
 
+               // Hook for extensions to say a title can't be moved for technical reasons
+               wfRunHooks( 'MovePageIsValidMove', array( $this->oldTitle, $this->newTitle, $status ) );
+
                return $status;
        }
 
@@ -126,6 +175,53 @@ class MovePage {
                return $status;
        }
 
+       /**
+        * Checks if $this can be moved to a given Title
+        * - Selects for update, so don't call it unless you mean business
+        *
+        * @since 1.25
+        * @return bool
+        */
+       protected function isValidMoveTarget() {
+               # Is it an existing file?
+               if ( $this->newTitle->inNamespace( NS_FILE ) ) {
+                       $file = wfLocalFile( $this->newTitle );
+                       if ( $file->exists() ) {
+                               wfDebug( __METHOD__ . ": file exists\n" );
+                               return false;
+                       }
+               }
+               # Is it a redirect with no history?
+               if ( !$this->newTitle->isSingleRevRedirect() ) {
+                       wfDebug( __METHOD__ . ": not a one-rev redirect\n" );
+                       return false;
+               }
+               # Get the article text
+               $rev = Revision::newFromTitle( $this->newTitle, false, Revision::READ_LATEST );
+               if ( !is_object( $rev ) ) {
+                       return false;
+               }
+               $content = $rev->getContent();
+               # Does the redirect point to the source?
+               # Or is it a broken self-redirect, usually caused by namespace collisions?
+               $redirTitle = $content ? $content->getRedirectTarget() : null;
+
+               if ( $redirTitle ) {
+                       if ( $redirTitle->getPrefixedDBkey() !== $this->oldTitle->getPrefixedDBkey() &&
+                               $redirTitle->getPrefixedDBkey() !== $this->newTitle->getPrefixedDBkey() ) {
+                               wfDebug( __METHOD__ . ": redirect points to other page\n" );
+                               return false;
+                       } else {
+                               return true;
+                       }
+               } else {
+                       # Fail safe (not a redirect after all. strange.)
+                       wfDebug( __METHOD__ . ": failsafe: database says " . $this->newTitle->getPrefixedDBkey() .
+                               " is a redirect, but it doesn't contain a valid redirect.\n" );
+                       return false;
+               }
+       }
+
        /**
         * @param User $user
         * @param string $reason
@@ -135,6 +231,8 @@ class MovePage {
        public function move( User $user, $reason, $createRedirect ) {
                global $wgCategoryCollation;
 
+               wfRunHooks( 'TitleMove', array( $this->oldTitle, $this->newTitle, $user ) );
+
                // If it is a file, move it first.
                // It is done before all other moving stuff is done because it's hard to revert.
                $dbw = wfGetDB( DB_MASTER );
@@ -274,7 +372,6 @@ class MovePage {
 
                wfRunHooks( 'TitleMoveComplete', array( &$this->oldTitle, &$this->newTitle, &$user, $pageid, $redirid, $reason ) );
                return Status::newGood();
-
        }
 
        /**
index 0efc94e..b97d36a 100644 (file)
@@ -2232,19 +2232,13 @@ class Title {
                } elseif ( $action == 'create' ) {
                        $title_protection = $this->getTitleProtection();
                        if ( $title_protection ) {
-                               if ( $title_protection['pt_create_perm'] == 'sysop' ) {
-                                       $title_protection['pt_create_perm'] = 'editprotected'; // B/C
-                               }
-                               if ( $title_protection['pt_create_perm'] == 'autoconfirmed' ) {
-                                       $title_protection['pt_create_perm'] = 'editsemiprotected'; // B/C
-                               }
-                               if ( $title_protection['pt_create_perm'] == ''
-                                       || !$user->isAllowed( $title_protection['pt_create_perm'] )
+                               if ( $title_protection['permission'] == ''
+                                       || !$user->isAllowed( $title_protection['permission'] )
                                ) {
                                        $errors[] = array(
                                                'titleprotected',
-                                               User::whoIs( $title_protection['pt_user'] ),
-                                               $title_protection['pt_reason']
+                                               User::whoIs( $title_protection['user'] ),
+                                               $title_protection['reason']
                                        );
                                }
                        }
@@ -2536,7 +2530,7 @@ class Title {
         * @return array|bool An associative array representing any existent title
         *   protection, or false if there's none.
         */
-       private function getTitleProtection() {
+       public function getTitleProtection() {
                // Can't protect pages in special namespaces
                if ( $this->getNamespace() < 0 ) {
                        return false;
@@ -2551,13 +2545,27 @@ class Title {
                        $dbr = wfGetDB( DB_SLAVE );
                        $res = $dbr->select(
                                'protected_titles',
-                               array( 'pt_user', 'pt_reason', 'pt_expiry', 'pt_create_perm' ),
+                               array(
+                                       'user' => 'pt_user',
+                                       'reason' => 'pt_reason',
+                                       'expiry' => 'pt_expiry',
+                                       'permission' => 'pt_create_perm'
+                               ),
                                array( 'pt_namespace' => $this->getNamespace(), 'pt_title' => $this->getDBkey() ),
                                __METHOD__
                        );
 
                        // fetchRow returns false if there are no rows.
-                       $this->mTitleProtection = $dbr->fetchRow( $res );
+                       $row = $dbr->fetchRow( $res );
+                       if ( $row ) {
+                               if ( $row['permission'] == 'sysop' ) {
+                                       $row['permission'] = 'editprotected'; // B/C
+                               }
+                               if ( $row['permission'] == 'autoconfirmed' ) {
+                                       $row['permission'] = 'editsemiprotected'; // B/C
+                               }
+                       }
+                       $this->mTitleProtection = $row;
                }
                return $this->mTitleProtection;
        }
@@ -2978,12 +2986,12 @@ class Title {
 
                                if ( $title_protection ) {
                                        $now = wfTimestampNow();
-                                       $expiry = $wgContLang->formatExpiry( $title_protection['pt_expiry'], TS_MW );
+                                       $expiry = $wgContLang->formatExpiry( $title_protection['expiry'], TS_MW );
 
                                        if ( !$expiry || $expiry > $now ) {
                                                // Apply the restrictions
                                                $this->mRestrictionsExpiry['create'] = $expiry;
-                                               $this->mRestrictions['create'] = explode( ',', trim( $title_protection['pt_create_perm'] ) );
+                                               $this->mRestrictions['create'] = explode( ',', trim( $title_protection['permission'] ) );
                                        } else { // Get rid of the old restrictions
                                                Title::purgeExpiredRestrictions();
                                                $this->mTitleProtection = false;
@@ -3579,10 +3587,12 @@ class Title {
        /**
         * Move this page without authentication
         *
+        * @deprecated since 1.25 use MovePage class instead
         * @param Title $nt The new page Title
         * @return array|bool True on success, getUserPermissionsErrors()-like array on failure
         */
        public function moveNoAuth( &$nt ) {
+               wfDeprecated( __METHOD__, '1.25' );
                return $this->moveTo( $nt, false );
        }
 
@@ -3590,10 +3600,9 @@ class Title {
         * Check whether a given move operation would be valid.
         * Returns true if ok, or a getUserPermissionsErrors()-like array otherwise
         *
-        * @todo finish moving this into MovePage
+        * @deprecated since 1.25, use MovePage's methods instead
         * @param Title $nt The new title
-        * @param bool $auth Indicates whether $wgUser's permissions
-        *  should be checked
+        * @param bool $auth Ignored
         * @param string $reason Is the log summary of the move, used for spam checking
         * @return array|bool True on success, getUserPermissionsErrors()-like array on failure
         */
@@ -3607,54 +3616,12 @@ class Title {
                }
 
                $mp = new MovePage( $this, $nt );
-               $errors = $mp->isValidMove()->getErrorsArray();
-
-               $newid = $nt->getArticleID();
-
-               if ( $auth ) {
-                       $errors = wfMergeErrorArrays( $errors,
-                               $this->getUserPermissionsErrors( 'move', $wgUser ),
-                               $this->getUserPermissionsErrors( 'edit', $wgUser ),
-                               $nt->getUserPermissionsErrors( 'move-target', $wgUser ),
-                               $nt->getUserPermissionsErrors( 'edit', $wgUser ) );
-               }
-
-               $match = EditPage::matchSummarySpamRegex( $reason );
-               if ( $match !== false ) {
-                       // This is kind of lame, won't display nice
-                       $errors[] = array( 'spamprotectiontext' );
-               }
-
-               $err = null;
-               if ( !wfRunHooks( 'AbortMove', array( $this, $nt, $wgUser, &$err, $reason ) ) ) {
-                       $errors[] = array( 'hookaborted', $err );
-               }
-
-               # The move is allowed only if (1) the target doesn't exist, or
-               # (2) the target is a redirect to the source, and has no history
-               # (so we can undo bad moves right after they're done).
+               $errors = wfMergeErrorArrays(
+                       $mp->isValidMove()->getErrorsArray(),
+                       $mp->checkPermissions( $wgUser, $reason )->getErrorsArray()
+               );
 
-               if ( 0 != $newid ) { # Target exists; check for validity
-                       if ( !$this->isValidMoveTarget( $nt ) ) {
-                               $errors[] = array( 'articleexists' );
-                       }
-               } else {
-                       $tp = $nt->getTitleProtection();
-                       $right = $tp['pt_create_perm'];
-                       if ( $right == 'sysop' ) {
-                               $right = 'editprotected'; // B/C
-                       }
-                       if ( $right == 'autoconfirmed' ) {
-                               $right = 'editsemiprotected'; // B/C
-                       }
-                       if ( $tp && !$wgUser->isAllowed( $right ) ) {
-                               $errors[] = array( 'cantmove-titleprotected' );
-                       }
-               }
-               if ( empty( $errors ) ) {
-                       return true;
-               }
-               return $errors;
+               return $errors ? : true;
        }
 
        /**
@@ -3679,7 +3646,7 @@ class Title {
        /**
         * Move a title to a new location
         *
-        * @todo Deprecate this in favor of MovePage
+        * @deprecated since 1.25, use the MovePage class instead
         * @param Title $nt The new title
         * @param bool $auth Indicates whether $wgUser's permissions
         *  should be checked
@@ -3701,8 +3668,6 @@ class Title {
                        $createRedirect = true;
                }
 
-               wfRunHooks( 'TitleMove', array( $this, $nt, $wgUser ) );
-
                $mp = new MovePage( $this, $nt );
                $status = $mp->move( $wgUser, $reason, $createRedirect );
                if ( $status->isOK() ) {
@@ -3838,7 +3803,7 @@ class Title {
         * Checks if $this can be moved to a given Title
         * - Selects for update, so don't call it unless you mean business
         *
-        * @todo move to MovePage
+        * @deprecated since 1.25, use MovePage's methods instead
         * @param Title $nt The new title to check
         * @return bool
         */
index db0fde3..1aa0d11 100644 (file)
@@ -72,9 +72,9 @@ class ApiMove extends ApiBase {
 
                // Move the page
                $toTitleExists = $toTitle->exists();
-               $retval = $fromTitle->moveTo( $toTitle, true, $params['reason'], !$params['noredirect'] );
-               if ( $retval !== true ) {
-                       $this->dieUsageMsg( reset( $retval ) );
+               $status = $this->movePage( $fromTitle, $toTitle, $params['reason'], !$params['noredirect'] );
+               if ( !$status->isOK() ) {
+                       $this->dieStatus( $status );
                }
 
                $r = array(
@@ -99,8 +99,8 @@ class ApiMove extends ApiBase {
                // Move the talk page
                if ( $params['movetalk'] && $fromTalk->exists() && !$fromTitle->isTalkPage() ) {
                        $toTalkExists = $toTalk->exists();
-                       $retval = $fromTalk->moveTo( $toTalk, true, $params['reason'], !$params['noredirect'] );
-                       if ( $retval === true ) {
+                       $status = $this->movePage( $fromTalk, $toTalk, $params['reason'], !$params['noredirect'] );
+                       if ( $status->isOK() ) {
                                $r['talkfrom'] = $fromTalk->getPrefixedText();
                                $r['talkto'] = $toTalk->getPrefixedText();
                                if ( $toTalkExists ) {
@@ -108,9 +108,9 @@ class ApiMove extends ApiBase {
                                }
                        } else {
                                // We're not gonna dieUsage() on failure, since we already changed something
-                               $parsed = $this->parseMsg( reset( $retval ) );
-                               $r['talkmove-error-code'] = $parsed['code'];
-                               $r['talkmove-error-info'] = $parsed['info'];
+                               $error = $this->getErrorFromStatus( $status );
+                               $r['talkmove-error-code'] = $error[0];
+                               $r['talkmove-error-info'] = $error[1];
                        }
                }
 
@@ -147,6 +147,28 @@ class ApiMove extends ApiBase {
                $result->addValue( null, $this->getModuleName(), $r );
        }
 
+       /**
+        * @param Title $from
+        * @param Title $to
+        * @param string $reason
+        * @param bool $createRedirect
+        * @return Status
+        */
+       protected function movePage( Title $from, Title $to, $reason, $createRedirect ) {
+               $mp = new MovePage( $from, $to );
+               $valid = $mp->isValidMove();
+               if ( !$valid->isOK() ) {
+                       return $valid;
+               }
+
+               $permStatus = $mp->checkPermissions( $this->getUser(), $reason );
+               if ( !$permStatus->isOK() ) {
+                       return $permStatus;
+               }
+
+               return $mp->move( $this->getUser(), $reason, $createRedirect );
+       }
+
        /**
         * @param Title $fromTitle
         * @param Title $toTitle
index ec9593f..4cbf584 100644 (file)
@@ -549,10 +549,22 @@ class MovePageForm extends UnlistedSpecialPage {
                }
 
                # Do the actual move.
-               $error = $ot->moveTo( $nt, true, $this->reason, $createRedirect );
-               if ( $error !== true ) {
-                       $this->showForm( $error );
+               $mp = new MovePage( $ot, $nt );
+               $valid = $mp->isValidMove();
+               if ( !$valid->isOK() ) {
+                       $this->showForm( $valid->getErrorsArray() );
+                       return;
+               }
+
+               $permStatus = $mp->checkPermissions( $user, $this->reason );
+               if ( !$permStatus->isOK() ) {
+                       $this->showForm( $permStatus->getErrorsArray() );
+                       return;
+               }
 
+               $status = $mp->move( $user, $this->reason, $createRedirect );
+               if ( !$status->isOK() ) {
+                       $this->showForm( $status->getErrorsArray() );
                        return;
                }
 
index 9e88c13..6234db4 100644 (file)
@@ -37,6 +37,9 @@ require_once __DIR__ . '/cleanupTable.inc';
  * @ingroup Maintenance
  */
 class CapsCleanup extends TableCleanup {
+
+       private $user;
+
        public function __construct() {
                parent::__construct();
                $this->mDescription = "Script to cleanup capitalization";
@@ -44,13 +47,13 @@ class CapsCleanup extends TableCleanup {
        }
 
        public function execute() {
-               global $wgCapitalLinks, $wgUser;
+               global $wgCapitalLinks;
 
                if ( $wgCapitalLinks ) {
                        $this->error( "\$wgCapitalLinks is on -- no need for caps links cleanup.", true );
                }
 
-               $wgUser = User::newFromName( 'Conversion script' );
+               $this->user = User::newFromName( 'Conversion script' );
 
                $this->namespace = intval( $this->getOption( 'namespace', 0 ) );
                $this->dryrun = $this->hasOption( 'dry-run' );
@@ -87,7 +90,9 @@ class CapsCleanup extends TableCleanup {
                        $this->output( "\"$display\" -> \"$targetDisplay\": DRY RUN, NOT MOVED\n" );
                        $ok = true;
                } else {
-                       $ok = $current->moveTo( $target, false, 'Converting page titles to lowercase' );
+                       $mp = new MovePage( $current, $target );
+                       $status = $mp->move( $this->user, 'Converting page titles to lowercase', true );
+                       $ok = $status->isOK() ? 'OK' : $status->getWikiText();
                        $this->output( "\"$display\" -> \"$targetDisplay\": $ok\n" );
                }
                if ( $ok === true ) {
index 713753f..a27a772 100644 (file)
@@ -103,10 +103,10 @@ class MoveBatch extends Maintenance {
 
                        $this->output( $source->getPrefixedText() . ' --> ' . $dest->getPrefixedText() );
                        $dbw->begin( __METHOD__ );
-                       $err = $source->moveTo( $dest, false, $reason, !$noredirects );
-                       if ( $err !== true ) {
-                               $msg = array_shift( $err[0] );
-                               $this->output( "\nFAILED: " . wfMessage( $msg, $err[0] )->text() );
+                       $mp = new MovePage( $source, $dest );
+                       $status = $mp->move( $wgUser, $reason, !$noredirects );
+                       if ( !$status->isOK() ) {
+                               $this->output( "\nFAILED: " . $status->getWikiText() );
                        }
                        $dbw->commit( __METHOD__ );
                        $this->output( "\n" );
index 49c0108..6af1862 100644 (file)
@@ -650,10 +650,10 @@ class TitlePermissionTest extends MediaWikiLangTestCase {
        public function testActionPermissions() {
                $this->setUserPerm( array( "createpage" ) );
                $this->setTitle( NS_MAIN, "test page" );
-               $this->title->mTitleProtection['pt_create_perm'] = '';
-               $this->title->mTitleProtection['pt_user'] = $this->user->getID();
-               $this->title->mTitleProtection['pt_expiry'] = wfGetDB( DB_SLAVE )->getInfinity();
-               $this->title->mTitleProtection['pt_reason'] = 'test';
+               $this->title->mTitleProtection['permission'] = '';
+               $this->title->mTitleProtection['user'] = $this->user->getID();
+               $this->title->mTitleProtection['expiry'] = wfGetDB( DB_SLAVE )->getInfinity();
+               $this->title->mTitleProtection['reason'] = 'test';
                $this->title->mCascadeRestriction = false;
 
                $this->assertEquals( array( array( 'titleprotected', 'Useruser', 'test' ) ),
@@ -661,7 +661,7 @@ class TitlePermissionTest extends MediaWikiLangTestCase {
                $this->assertEquals( false,
                        $this->title->userCan( 'create', $this->user ) );
 
-               $this->title->mTitleProtection['pt_create_perm'] = 'sysop';
+               $this->title->mTitleProtection['permission'] = 'editprotected';
                $this->setUserPerm( array( 'createpage', 'protect' ) );
                $this->assertEquals( array( array( 'titleprotected', 'Useruser', 'test' ) ),
                        $this->title->getUserPermissionsErrors( 'create', $this->user ) );